Implement new overload compatibility checking #6075

DanielRosenwasser · 2015-12-12T00:22:11Z

Addresses #943 and #4180.

This PR loosens the compatibility rules between an implementation and an overload. We now check for both

whether the implementation is assignable to the overload, while disregarding return types
whether the implementation return type is assignable to or from the overload return type

This means that something like

function getAnimal(x: number): Dog;
function getAnimal(x: string): Cat;
function getAnimal(x: number | string): Cat | Dog {
    // ...
}

will finally work, as will

function getAnimal(x: number): Dog;
function getAnimal(x: string): Cat;
function getAnimal(x: number | string): Animal {
    // ...
}

…ementations.

… than what was guaranteed.

…more general than the overload.

…ntation signature for every overload.

JsonFreeman · 2015-12-13T20:24:48Z

src/compiler/checker.ts

+            const targetReturnType = getReturnTypeOfSignature(erasedTarget);
+            if (targetReturnType === voidType
+                || checkTypeRelatedTo(targetReturnType, sourceReturnType, assignableRelation, /*errorNode*/ undefined)
+                || checkTypeRelatedTo(sourceReturnType, targetReturnType, assignableRelation, /*errorNode*/ undefined)) {


In response to @weswigham's point #943 (comment), I believe the special casing of signature return types wouldn't come into play here. The return types are being compared solely as types, and not signature return types. So I think boolean would be assignable to a type predicate in this context, which is indeed desirable.

Exactly, and you can see that in stringLiteralTypesAsTags01.ts. I'll have to add more test cases though.

My ultimate point is that concern number 2 in #943 (comment) is not an issue, since boolean would work in the /*????*/ slot.

ahejlsberg · 2015-12-16T00:05:27Z

src/compiler/checker.ts

+                // Create object types to actually perform relation checks.
+                const anyReturningSourceType = getOrCreateTypeFromSignature(anyReturningSource);
+                const anyReturningTargetType = getOrCreateTypeFromSignature(anyReturningTarget);
+                return checkTypeRelatedTo(anyReturningSourceType, anyReturningTargetType, assignableRelation, /*errorNode*/ undefined);


We're computing and caching an awful lot of types here. They'll sit in the cache of their associated signature forever and also gum up the assignable relation cache. I think we need a function similar to compareSignatures that checks for assignability instead of identity. The function could have an ignoreReturnTypes flag and then you wouldn't need to create all of these new signatures and types.

Agreed.

One other possibility is to just call isSignatureAssignableTo in both directions. For the return types, doing this would yield the bivariance you are seeking. Parameter types are bivariant anyway, so each corresponding pair would just get checked in both directions twice. The only real difference is that you'd be extra permissive with the arity checks on the signatures. Namely, an overload would be allowed to be shorter (fewer parameters) than the implementation signature. You could add the necessary arity checks after the comparison to make sure the arities are only related in the direction you want. It's kind of nifty, though perhaps a bit more roundabout than the compareSignatures style approach.

isSignatureAssignableTo was already doing the work I was trying to avoid, so I might as well just avoid the caching.

I've pushed out a change that does as @ahejlsberg's requested. There's no caching and no new type creation.

…nature.

JsonFreeman · 2015-12-16T04:10:08Z

src/compiler/checker.ts

            function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary {
+                // TODO (drosen): De-duplicate code between related functions.


Yes, this is concerning. There is a lot of duplicated functionality here.

I have a separate branch for that work right now, but I'd like to get it in as part of a separate PR.

It looks like it's mostly down to isRelatedTo vs isSignatureAssignableTo and Ternary vs boolean. Am I missing something?

sandersn · 2015-12-17T22:21:09Z

src/compiler/checker.ts

+                }
+                const sourceReturnType = getReturnTypeOfSignature(source);
+
+                // The following block preserves behavior forbidding boolean returning functions from being assignable to type guard returning functions


I think you can use fewer words here: Just say "Preserve behaviour forbidding boolean functions from being assignable to type guards."

Might as well change the original in signatureRelatedTo also

sandersn · 2015-12-17T22:46:00Z

Looks good enough to merge if the de-dupe cleanup is coming soon. The semantics a bit loose for my liking but they are more consistent with existing Typescript.

DanielRosenwasser · 2015-12-18T01:15:15Z

If you'd like to discuss the semantics tomorrow, I'm open to it. 1.8 isn't set in stone just yet.

Implement new overload compatibility checking

DanielRosenwasser added 17 commits December 8, 2015 14:28

Fixed test to explicitly have return type annotation.

2cc6c4c

Accepted baselines.

5ef8a30

Remove empty test.

2c2b8be

Added some tests for overload compatibility.

820a2cb

Accepted baselines.

5d94e48

Use different relation for overload compatibility.

dc2af2f

Removed misleading comment from test.

e1004fa

Accepted baselines.

51d2409

Merge branch 'master' into overloadCompatibility

fc170da

Fixed unnecessary error in test.

e9dc011

Accepted baselines.

8cceedd

Added tests for 'void' return type compatibilty on overloads and impl…

3cdfc36

…ementations.

Accepted baselines.

069fada

Specifically test for 'void' to permit implementations to return more…

9b507b7

… than what was guaranteed.

Accepted baselines.

e012645

Reversed order of checks, since the implementation will typically be …

a236461

…more general than the overload.

Do some caching so that we don't repeat the same work for the impleme…

73526cf

…ntation signature for every overload.

msftclas added the cla-not-required label Dec 12, 2015

DanielRosenwasser mentioned this pull request Dec 13, 2015

Use different relation when checking overload vs implementation signature compatibility #943

Closed

JsonFreeman reviewed Dec 13, 2015
View reviewed changes

DanielRosenwasser added 2 commits December 14, 2015 14:16

Added tests.

5cbf17d

Accepted baselines.

2efa697

ahejlsberg reviewed Dec 16, 2015
View reviewed changes

Use a comparison function instead of creating a new type for each sig…

d8db60a

…nature.

JsonFreeman reviewed Dec 16, 2015
View reviewed changes

Fix linter nits.

731925b

sandersn reviewed Dec 17, 2015
View reviewed changes

DanielRosenwasser added a commit that referenced this pull request Dec 18, 2015

Merge pull request #6075 from Microsoft/overloadCompatibility

8d46ffd

Implement new overload compatibility checking

DanielRosenwasser merged commit 8d46ffd into master Dec 18, 2015

DanielRosenwasser deleted the overloadCompatibility branch December 18, 2015 01:15

DanielRosenwasser mentioned this pull request Dec 18, 2015

Refactor signature relatability #6142

Merged

DanielRosenwasser mentioned this pull request Dec 28, 2015

Remove most special treatment of specialized signatures. #6278

Merged

sandersn mentioned this pull request Apr 22, 2016

String Literal issue with function overloads #8251

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new overload compatibility checking #6075

Implement new overload compatibility checking #6075

DanielRosenwasser commented Dec 12, 2015

JsonFreeman Dec 13, 2015

DanielRosenwasser Dec 13, 2015

JsonFreeman Dec 13, 2015

ahejlsberg Dec 16, 2015

JsonFreeman Dec 16, 2015

DanielRosenwasser Dec 16, 2015

DanielRosenwasser Dec 16, 2015

JsonFreeman Dec 16, 2015

DanielRosenwasser Dec 16, 2015

sandersn Dec 17, 2015

sandersn Dec 17, 2015

sandersn Dec 17, 2015

sandersn commented Dec 17, 2015

DanielRosenwasser commented Dec 18, 2015

		function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary {
		// TODO (drosen): De-duplicate code between related functions.

Implement new overload compatibility checking #6075

Implement new overload compatibility checking #6075

Conversation

DanielRosenwasser commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Dec 17, 2015

DanielRosenwasser commented Dec 18, 2015